Drop zone: rewrite with hooks (useDropZone) #19514

ellatrix · 2020-01-08T20:12:57Z

Description

This PR removes the last remaining control element from the block DOM. It looks like it offers a further 10% performance improvement on typing.

I created a useDropZone hook alternative that allows you to create a drop zone without needing to create an additional element. It works by passing the reference to the target element to the hook.

How has this been tested?

Move blocks by drag and drop.
Drop a block on an appender.
Drop files between blocks.
Drop files on the media placeholder.

Screenshots

Types of changes

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR. .

ellatrix · 2020-01-09T10:36:48Z

Performance test:


 + 69f98a0db...e8bf37d4e try/drop-zone-hooks -> try/drop-zone-hooks (forced update)
Ellas-MacBook-Pro:gutenberg ella$ npm run test-performance

> gutenberg@7.2.0 test-performance /Users/ella/gutenberg
> wp-scripts test-e2e --config packages/e2e-tests/jest.performance.config.js

 PASS  packages/e2e-tests/specs/performance/performance.test.js (23.948s)
  Performance
    ✓ 1000 paragraphs (20627ms)

Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        24.067s, estimated 26s
Ran all test suites.

Average time to load: 5149ms
Average time to DOM content load: 4832ms
Average time to type character: 40.655ms
Slowest time to type character: 127ms
Fastest time to type character: 29ms
		
Ellas-MacBook-Pro:gutenberg ella$ git checkout master
M	packages/e2e-tests/config/setup-test-framework.js
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
Ellas-MacBook-Pro:gutenberg ella$ npm run test-performance

> gutenberg@7.2.0 test-performance /Users/ella/gutenberg
> wp-scripts test-e2e --config packages/e2e-tests/jest.performance.config.js

 PASS  packages/e2e-tests/specs/performance/performance.test.js (25.898s)
  Performance
    ✓ 1000 paragraphs (23110ms)

Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        26.01s
Ran all test suites.

Average time to load: 5531ms
Average time to DOM content load: 5143ms
Average time to type character: 45.255ms
Slowest time to type character: 125ms
Fastest time to type character: 33ms
		
Ellas-MacBook-Pro:gutenberg ella$

youknowriad

When you start a new post, dropping a file on the post placeholder doesn't work.

youknowriad · 2020-01-10T08:35:26Z

packages/block-editor/src/components/block-drop-zone/index.js

+		getClientIdsOfDescendants,
+		hasUploadPermissions,
+		isLockedAll,
+	} = useSelect( selector, [ rootClientId ] );


What pattern did we settle on? I thought we decided to use inline functions?

I thought we settled on allowing it. It's especially handy if variable names are clashing. It's also an optimisation if useSelect has no dependencies, you can move it out of the component. Here we depend on rootClientId though.

packages/block-editor/src/components/block-list/block.js

packages/block-editor/src/components/block-list-appender/index.js

packages/block-editor/src/components/button-block-appender/index.js

packages/block-editor/src/components/default-block-appender/index.js

youknowriad · 2020-01-10T08:44:53Z

packages/components/src/drop-zone/index.js

-import { DropZoneConsumer } from './provider';
+import { DropZoneConsumer, Context } from './provider';
+
+export function useDropZone( { element, onFilesDrop, onHTMLDrop, onDrop, isDisabled } ) {


Should we add some JSDocs and types :p cc @aduth

Should we move this to @wordpress/compose like all other hooks? I guess the problem is that it's dependent on the DropzoneProvider so I'm on the fence. I know the provider is necessary for performance reasons, other than that it would be good to kill.

I haven't looked into the DropzoneProvider, but as long as we need that, this seems like the right place. We can prefix it with __unstable if we want to keep our options open.

I prefixed it. I will have another look separately to move it and look into the provider.

youknowriad · 2020-01-10T08:53:27Z

packages/block-editor/src/components/block-list/block.js

-			{ shouldRenderDropzone && <BlockDropZone
-				clientId={ clientId }
-				rootClientId={ rootClientId }
-			/> }


The main problem with this PR I think is that the dropzone is smaller than before. In master for paragraphs, its size is "37px" with this pr it is "30px". They both suffer from the same issue, there's a space (margin) between blocks where you can't drop blocks. It's is more visible in this PR because of the smaller size though.

@youknowriad I fixed the issue entirely by rewriting useBlockDropZone to work on BlockList instead of BlockListBlock. Now there are no longer "dead zones" between the blocks where you can't drop.

This is cool, I'll test a bit later.

Thanks! I think the error might be gone too, but let me know if it isn't. :)

youknowriad · 2020-01-10T08:58:08Z

After a few drag and drops, I started having this JS error while dragging blocks.

ellatrix · 2020-01-10T09:11:39Z

@youknowriad Thanks, I'll fix the issues!

youknowriad · 2020-01-10T11:53:36Z

packages/block-editor/src/components/block-drop-zone/index.js

+			const offset = y - rect.top;
+			const target = Array.from( element.current.children ).find( ( blockEl ) => {
+				return blockEl.offsetTop + ( blockEl.offsetHeight / 2 ) > offset;
+			} );


Is it possible to take into consideration the blocks that are laid out horizontally (I'm not sure it was the case before)

It's not possible in master right now, so I'd prefer to tackle this in a separate PR to keep this small and contained.

youknowriad · 2020-01-10T11:55:04Z

packages/components/src/drop-zone/provider.js

+			let x = detail.clientX;
+			let y = detail.clientY;
+
+			if ( ! hoveredDropZone.withExactPosition ) {


should we just use another property like "coordinates" instead of hacking two different types in the same property?

I personally wouldn't, because it will make the component rerender much more, but I'm fine with it either way. We could remove the top/bottom calculation since they were specific for blocks.

Although is-close-to-top is part of DropZone's API. Wouldn't mind removing it if it's ok, nothing in Gutenberg is now using these classes.

I don't mind dropping it too if not used. (dev note)

Ok, I removed them. :)

youknowriad · 2020-01-10T13:37:18Z

Usability wise, this feels way better than master. I'd appreciate some testing from @jasmussen

jasmussen

Trusting the code review to Riad and others, from an experience point of view this is substantially better. Before:

After:

ellatrix · 2020-01-10T15:39:52Z

Thanks a lot for the reviews! Let's merge and iterate. I think this improves things in many ways.

chrisvanpatten · 2020-01-10T15:50:15Z

I am so excited for this PR — the old drag-and-drop behavior was quite finicky but this looks like a tremendous improvement!

aduth · 2020-01-10T16:19:01Z

When you start a new post, dropping a file on the post placeholder doesn't work.

I don't know if this was addressed, but I'm pretty sure it behaved this way (i.e. didn't allow drop on a new post) prior to the changes here.

ellatrix · 2020-01-11T14:26:36Z

I double checked that you can drop on the appender, but right, you can't drop on the rest of the document.

plaidpowered · 2020-02-27T21:45:47Z

The dropzone behavior is so much more stable with this change, but I have to ask, is the editor.BlockDropZone coming back? Or is there some way to attach an equivalent behavior hook?

ellatrix · 2020-02-28T11:49:47Z

I think we need to see how media upload can best be filtered. I think the drop zone is the wrong place for a hook because (1) there’s more places where media can be uploaded/inserted (paste, placeholder…) and (2) the drop zone handles more than just media, it also handles internal block dropping. @plaidpowered What were you using the hook for?

plaidpowered · 2020-02-28T12:32:46Z

Thanks for getting back to me!
I was essentially using the filter to take over the dropzone behavior and add more functionality - for instance, give the user the option to swap two blocks, rather than move/insert .

Thankfully no production code has been broken, but as the project was in progress, I wanted to ensure my work is future-proof. I may have to go at it from a different direction.

ellatrix · 2020-02-28T12:36:23Z

@plaidpowered Would be good to see it in action with a GIF for example, to better understand what you mean. Let me know if there's anything in particular you'd like to filter after this change. Any props in useBlockDropZone?

plaidpowered · 2020-02-28T13:27:07Z

Actually, perhaps even better than a filter would be to add an action at the end of onDrop, something like

wp.hooks.doAction( 'editor.BlockDropZone.afterOnDrop', src, dst, event );

For my specific use case, that's literally all I'd need. Of course, this wouldn't filter onDrop behavior, simply allow actions to be performed after blocks are rearranged, and I imagine there would be multiple use cases beyond my own. For instance, if a developer wants to validate whether a block is allowed to be placed in a certain location.

I really appreciate you discussing this with me, thanks!

ellatrix · 2020-02-28T14:09:04Z

What are you doing after the blocks have rearranged? I don't get what this "option to swap two blocks" is. :)

plaidpowered · 2020-02-28T14:28:02Z

It depends on a number of factors, some of which I haven't figured out yet - the idea is that if you have block a and block b, and drag block a onto block b, block a will take the location of block b and block b will be moved to the former location of block a.

I'm still working out the UI for this. Ideally I want a dedicated area in block b's dropzone, and if the block a is dropped there, the replacement/swap occurs instead of the standard insert behavior. But if I can't make that work with the present codebase, I'll present some kind of dialog to the user asking if they want to Insert (before the dst block), Swap (dst with src), or Replace (dst is deleted, src takes its place).

Admittedly, the old editor.BlockDropZone filter wasn't very helpful in reaching this goal, essentially I had to completely replace dropZone.prototype.onDrop from within the addFilter callback. That never felt like an effective long term solution.

ellatrix · 2020-02-28T15:04:30Z

The dragging and dropping of blocks already swaps the blocks, no?

plaidpowered · 2020-02-28T15:15:17Z

I suppose it does if you are dragging and dropping blocks within the same parent container, and the two blocks are adjacent to each other, but if you are moving blocks between two different containers, the present behavior simply inserts the dragged block to before the destination dropzone. My intention is to have the dragged block and destination block trade places, even if they are in different containers.

plaidpowered · 2020-02-28T19:46:14Z

Hi again @ellatrix
I was able to accomplish my goal through a different method, that avoids using actions/filters - using the dragover/dragend window events, and the wp.data store. Here's a GIF of what I was talking about.

Anyway, thanks for your help! It may still be valuable to include some filters in this functionality. Filters are the best. :D

ellatrix added [Type] Performance Related to performance efforts [Type] Code Quality Issues or PRs that relate to code quality labels Jan 8, 2020

ellatrix marked this pull request as ready for review January 8, 2020 22:33

ellatrix requested review from ajitbohra, chrisvanpatten, jorgefilipecosta, talldan and youknowriad as code owners January 8, 2020 22:33

ellatrix force-pushed the try/drop-zone-hooks branch from 69f98a0 to e8bf37d Compare January 9, 2020 10:32

ellatrix changed the title ~~Drop zone: rewrite with hooks~~ Drop zone: rewrite with hooks (useDropZone) Jan 9, 2020

ellatrix added 8 commits January 10, 2020 09:01

Drop zone: rewrite with hooks

b082ca6

Add missing drop zones

413e2aa

Use hook for component

66e6b91

Oops

464e660

fix styling

4032990

fix unit tests

af10ad6

Simplify

3868c40

Add style for appender drop target

9d3667b

ellatrix force-pushed the try/drop-zone-hooks branch from e8bf37d to 9d3667b Compare January 10, 2020 08:01

Readd className prop

4011f05

youknowriad reviewed Jan 10, 2020

View reviewed changes

ellatrix added 3 commits January 10, 2020 12:00

Add dropzone to block list instead of block

5f1fc4f

Add appender style

a3f8278

Clean up

b1f2474

swissspidy mentioned this pull request Jan 10, 2020

Page reordering from carousel ampproject/amp-wp#4048

Merged

3 tasks

youknowriad reviewed Jan 10, 2020

View reviewed changes

Remove position classes

257a093

ellatrix force-pushed the try/drop-zone-hooks branch from 4c4b22a to 257a093 Compare January 10, 2020 14:06

ellatrix added the Needs Dev Note Requires a developer note for a major WordPress release cycle label Jan 10, 2020

jasmussen self-requested a review January 10, 2020 14:44

jasmussen approved these changes Jan 10, 2020

View reviewed changes

ellatrix merged commit 1e0a6ce into master Jan 10, 2020

ellatrix deleted the try/drop-zone-hooks branch January 10, 2020 15:39

ellatrix added this to the Gutenberg 7.3 milestone Jan 20, 2020

jorgefilipecosta mentioned this pull request Feb 12, 2020

Block Editor Dev Notes 5.4 #20185

Closed

23 tasks

herregroen mentioned this pull request Mar 26, 2020

The annotations package breaks drop targets when dragging blocks #21172

Closed

aduth mentioned this pull request Mar 26, 2020

Annotations: Merge assigned block className with incoming prop #21184

Merged

youknowriad removed the Needs Dev Note Requires a developer note for a major WordPress release cycle label Apr 8, 2020

This was referenced May 26, 2020

editor.BlockDropZone filter removed #22618

Closed

Move BlockDropZone react hook and update docs #22619

Merged

ellatrix mentioned this pull request Jun 29, 2020

DropZone component no longer provides x and y position #23149

Closed

Drop zone: rewrite with hooks (useDropZone) #19514

Drop zone: rewrite with hooks (useDropZone) #19514

Conversation

ellatrix commented Jan 8, 2020 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

ellatrix commented Jan 9, 2020

youknowriad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Jan 10, 2020 • edited Loading

ellatrix commented Jan 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Jan 10, 2020

jasmussen left a comment

Choose a reason for hiding this comment

ellatrix commented Jan 10, 2020

chrisvanpatten commented Jan 10, 2020

aduth commented Jan 10, 2020 • edited Loading

ellatrix commented Jan 11, 2020

plaidpowered commented Feb 27, 2020

ellatrix commented Feb 28, 2020

plaidpowered commented Feb 28, 2020 • edited Loading

ellatrix commented Feb 28, 2020

plaidpowered commented Feb 28, 2020

ellatrix commented Feb 28, 2020

plaidpowered commented Feb 28, 2020 • edited Loading

ellatrix commented Feb 28, 2020

plaidpowered commented Feb 28, 2020

plaidpowered commented Feb 28, 2020 • edited Loading

ellatrix commented Jan 8, 2020 •

edited

Loading

youknowriad commented Jan 10, 2020 •

edited

Loading

aduth commented Jan 10, 2020 •

edited

Loading

plaidpowered commented Feb 28, 2020 •

edited

Loading

plaidpowered commented Feb 28, 2020 •

edited

Loading

plaidpowered commented Feb 28, 2020 •

edited

Loading